Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid heap corruption issue by setting the property via the DP instead of the property setter #5781

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

StephenLPeters
Copy link
Contributor

An OS bug was fixed in 20h1 that resolved the heap corruption bug our code is hitting on 19h1 and 19h2. Fixes #5435

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Aug 27, 2021
@StephenLPeters
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor Author

/AZP RUN

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-Materials Reveal, Acrylic, lighting, etc. team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 8, 2021
@@ -63,6 +64,7 @@ class SharedHelpers

// Actual OS version checks
static bool IsAPIContractV14Available(); // 21H1
static bool IsAPIContractV10Available(); // 20H1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static bool IsAPIContractV10Available(); // 20H1
static bool IsAPIContractV10Available(); // 20H1

{
// On 19h1 and 19h2 there is a heap corruption bug that occurs when setting the TintLuminosityOpacity via the property,
// So we'll set it by the DP instead.
brush.SetValue(winrt::Windows::UI::Xaml::Media::AcrylicBrush::TintLuminosityOpacityProperty(), box_value(luminosityValue));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the only difference is that this doesn't trigger the "notify on property change" handlers, which is probably ok for this prop but would be worth adding a comment for

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches my understanding of the issue and the proposal that I had made in the internal email investigation thread we had. Thank you for doing this! Hopefully we can get a release with this fix very soon!

@StephenLPeters StephenLPeters merged commit f8c9591 into main Sep 10, 2021
@StephenLPeters StephenLPeters deleted the user/stpete/TintLuminocityHeapCorruption branch September 10, 2021 17:29
@StephenLPeters StephenLPeters restored the user/stpete/TintLuminocityHeapCorruption branch December 2, 2021 03:19
StephenLPeters added a commit that referenced this pull request Dec 2, 2021
…d of the property setter (#5781)

* Avoid heap corruption issue by setting the property via the DP instead of the property setter.

* Disable setting the property on 19h1 and 19h2

* reenable set by DP to test with repo project
@ghost
Copy link

ghost commented Dec 3, 2021

🎉Microsoft.UI.Xaml v2.6.3 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Materials Reveal, Acrylic, lighting, etc. team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using WinUI 2.6.1 in a .net core 3.1 app (XAML island) crashes at startup on older Windows version
3 participants